Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix removeInputsFromSelection to only remove inputs #2859

Merged
merged 5 commits into from
Nov 15, 2023
Merged

Conversation

clenfest
Copy link
Contributor

removeInputsFromSelection is tasked with removing inputs to a FetchGroup from the fetch group so that we don't retrieve the same values twice (and FetchGroups that don't functionally do anything can be removed). There was a bug in the code that meant that in certain cases, things that were required were actually being removed. This could manifest in subgraph jumps not being possible because the key it was using to make the jump was not available.

@clenfest clenfest requested a review from a team as a code owner November 14, 2023 22:59
Copy link

changeset-bot bot commented Nov 14, 2023

🦋 Changeset detected

Latest commit: 1ff5e3b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@apollo/query-planner Patch
@apollo/gateway Patch
@apollo/federation-internals Patch
@apollo/composition Patch
@apollo/query-graphs Patch
@apollo/subgraph Patch
apollo-federation-integration-testsuite Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Nov 14, 2023

Deploy Preview for apollo-federation-docs ready!

Name Link
🔨 Latest commit 1ff5e3b
🔍 Latest deploy log https://app.netlify.com/sites/apollo-federation-docs/deploys/65543db8f8aca70008e8b412
😎 Deploy Preview https://deploy-preview-2859--apollo-federation-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codesandbox-ci bot commented Nov 14, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice sleuthing!

@@ -3810,7 +3810,7 @@ describe('Fed1 supergraph handling', () => {
});

describe('Named fragments preservation', () => {
it('works with nested fragments', () => {
it('works with nested fragments 1', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stray?

Suggested change
it('works with nested fragments 1', () => {
it('works with nested fragments', () => {

Copy link
Contributor Author

@clenfest clenfest Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I had this in here is that if you try to run a test by name, jest will actually run all tests for which that name is a substring. So if you run "test1", but a "test11" and "test12" exist, it will run all three. I was trying to debug this test, and got confused because another test would run immediately afterwards, and it took me a while to realize it was a separate execution.

.changeset/pink-ducks-fail.md Outdated Show resolved Hide resolved
@clenfest clenfest enabled auto-merge (squash) November 15, 2023 03:41
@clenfest clenfest merged commit a0bdd7c into main Nov 15, 2023
16 checks passed
@clenfest clenfest deleted the clenfest/fp-513 branch November 15, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants